Properly handle non-approved revision deletions

kitsune/wiki/handlers.py

@@ -11,30 +11,38 @@ class DocumentListener(UserDeletionListener):
     """Listener for document-related tasks."""
 
     def on_user_deletion(self, user: User) -> None:
-        """Handle the deletion of a user."""
+        """Handle the deletion of a user.
 
-        documents = Document.objects.filter(contributors=user)
+        - Replace the user as a document contributor with content group members
+          if they were the only contributor.
+        - Anonymize the user's revision votes.
+        - Delete the user's non-approved revisions.
+        - Replace the user as creator/reviewer of approved revisions with the SUMO bot.
+        """
+        sumo_bot = Profile.get_sumo_bot()
         content_group = Group.objects.get(name=settings.SUMO_CONTENT_GROUP)
-        for document in documents:
-            if not document.contributors.exclude(id=user.id).exists():
-                document.contributors.add(*content_group.user_set.all())
-            document.contributors.remove(user)
-
-        non_approved_revisions = Revision.objects.filter(creator=user, is_approved=False)
-        # Delete documents with no approved revisions
-        Document.objects.filter(
-            current_revision__isnull=True, revisions__in=non_approved_revisions
-        ).delete()
-        # delete remaining non-approved revisions
-        non_approved_revisions.delete()
 
-        sumo_bot = Profile.get_sumo_bot()
-        Revision.objects.filter(creator=user).update(creator=sumo_bot)
+        # Handle approved revisions first to avoid cascade deletion issues
+        Revision.objects.filter(creator=user, is_approved=True).update(creator=sumo_bot)
+        # Update reviewer field
         Revision.objects.filter(reviewer=user).update(reviewer=sumo_bot)
+        # Update readied_for_localization_by field separately
         Revision.objects.filter(readied_for_localization_by=user).update(
             readied_for_localization_by=sumo_bot
         )
-        # Anonymize any revision votes.
+
+        documents = Document.objects.filter(contributors=user)
+        for document in documents:
+            if not document.contributors.exclude(id=user.id).exists():
+                document.contributors.add(*content_group.user_set.all())
+            document.contributors.remove(user)
+
         HelpfulVote.objects.filter(creator=user).update(
             creator=None, anonymous_id=AnonymousIdentity().anonymous_id
         )
+
+        Document.objects.filter(
+            revisions__creator=user,
+            current_revision__isnull=True,
+        ).exclude(revisions__creator__in=User.objects.exclude(id=user.id)).delete()
+        Revision.objects.filter(creator=user, is_approved=False).delete()

kitsune/wiki/tests/test_handlers.py

@@ -152,18 +152,56 @@ def test_revision_reviewer_replacement(self):
             reviewer=self.user,
             readied_for_localization_by=self.user,
         )
+        # Add a revision that has a reviewer but no readied_for_localization_by
+        rev4 = ApprovedRevisionFactory(
+            creator=contributor,
+            reviewer=self.user,
+        )
 
         self.listener.on_user_deletion(self.user)
 
-        for rev in (rev1, rev2, rev3):
+        for rev in (rev1, rev2, rev3, rev4):
             rev.refresh_from_db()
 
         self.assertEqual(rev1.creator.username, contributor.username)
         self.assertEqual(rev1.reviewer.username, reviewer.username)
         self.assertEqual(rev1.readied_for_localization_by.username, reviewer.username)
+
         self.assertEqual(rev2.creator.username, settings.SUMO_BOT_USERNAME)
         self.assertEqual(rev2.reviewer.username, reviewer.username)
         self.assertEqual(rev2.readied_for_localization_by.username, reviewer.username)
+
         self.assertEqual(rev3.creator.username, contributor.username)
         self.assertEqual(rev3.reviewer.username, settings.SUMO_BOT_USERNAME)
         self.assertEqual(rev3.readied_for_localization_by.username, settings.SUMO_BOT_USERNAME)
+
+        self.assertEqual(rev4.creator.username, contributor.username)
+        self.assertEqual(rev4.reviewer.username, settings.SUMO_BOT_USERNAME)
+        self.assertIsNone(rev4.readied_for_localization_by)
+
+    def test_mixed_revisions_user_deletion(self):
+        """
+        Test that when a user is deleted, only their unapproved revisions are deleted,
+        and documents with revisions from other users are preserved.
+        """
+        doc = DocumentFactory(current_revision=None)
+
+        other_user = UserFactory()
+        rev1 = RevisionFactory(document=doc, creator=other_user, is_approved=False)
+
+        rev2 = RevisionFactory(document=doc, creator=self.user, is_approved=False)
+
+        doc_id = doc.id
+        rev1_id = rev1.id
+        rev2_id = rev2.id
+
+        self.listener.on_user_deletion(self.user)
+
+        doc = Document.objects.filter(id=doc_id).first()
+        self.assertIsNotNone(doc, "Document should not be deleted")
+
+        rev1 = Revision.objects.filter(id=rev1_id).first()
+        self.assertIsNotNone(rev1, "Other user's revision should not be deleted")
+
+        rev2 = Revision.objects.filter(id=rev2_id).first()
+        self.assertIsNone(rev2, "Deleted user's revision should be deleted")